-
-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: impl TryFrom str for Timestamp #137
Conversation
Signed-off-by: tison <[email protected]>
d76fc34
to
c68e9a1
Compare
Or I can add a wrapper type The former can enable the |
Adopt #137 (comment) so not an necessary patch for me. Close since it doesn't seems follow the conversion flavor. But I'd still like to know the reason and if this is valid, we can reopen anyway. |
The missing But popping up a level, your stated motivation is not quite right:
The API you've provided, regardless of whether you accept |
As for the motivation of making the call site more succinct, there are a variety of ways to tackle that. Jiff itself uses various techniques that you might learn from. |
As supporting other And yes, re-export is possible, while I'm not sure the range of the exported struct so I try to use an alternative.
Yes. I saw some of them. But I can't sort them out currently .. Would you point me some typical methods or is there any existing note on solving this kind of requirement? |
Sorry, it's very hard for me to understand your English. For example, I don't know what you mean by "I'm not sure the range of the exported struct."
Like I said in the other issue, I don't really have a ton of time to do abstract mentoring. It sounds like you already learned on approach. There are other approaches with varying trade offs. Like custom traits. API design is a very big topic, and there just aren't readily accessible materials that I'm aware of that explain everything. |
Got it and thanks for all your time. |
In trying to improve the expression ... The first thing I tried was re-export This may sound like unfounded worry, but I'm still learning the boundaries and best practices. Since using a wrapper fits my use case, I'm using it instead. #[derive(Debug, Copy, Clone)]
pub struct MakeTimestamp(pub Timestamp);
impl From<Timestamp> for MakeTimestamp { ... }
impl FromStr for MakeTimestamp { ... }
impl<'a> TryFrom<&'a str> for MakeTimestamp { ... }
impl MakeTimestamp {
pub fn from_second(second: i64) -> Result<Self, Error> { ... }
pub fn from_millisecond(millisecond: i64) -> Result<Self, Error> { ... }
pub fn from_microsecond(microsecond: i64) -> Result<Self, Error> { ... }
pub fn from_nanosecond(nanosecond: i128) -> Result<Self, Error> { ... }
}
I got you now. You mean the API implemented in this PR, and that's correct. With This can be subtly confusing also. So you're right that the way proposed at the beginning is not quite right. |
For re-exporting, you should be doing
No... I mean your API you have as a motivating example. You're returning a Zoned. Therefore, Jiff is already a public dependency by virtue of that alone. The TryInto or whatever is irrelevant. And again, as long as you re-export the entire Jiff crate, which is standard practice for public dependencies, callers don't need to add an explicit dependency on Jiff. See https://www.lurklurk.org/effective-rust/re-export.html and rust-lang/api-guidelines#176 Of course, it would be better not to make Jiff a public dependency in the first place. So you're right to chase wrapper types for Timestamp and what not. But your motivating example would not avoid the public dependency on Jiff because you still have a return type that references an item in Jiff's API. Maybe you're confused about what a public dependency is. I'm not sure. Basically, if Jiff types appear anywhere in the API of your library, then Jiff is a public dependency. |
Oops .. Yes. As I return a And thanks for the references you linked :D |
Not quite sure if this falls into our API philosophy.
My use case is such a method (full code):
... and I want
foo.find_next("2024-01-01T00:00:00+08:00")
compile.The reason for such a method is:
Alternatives. I can switch to:
|| "...".parse()
and|| Ok(ts)
are accepted.